Skip to content

Refactor ECS scheduled tasks#1992

Draft
theseanything wants to merge 3 commits intomainfrom
theseanything/add-ecs-scheduled-tasks
Draft

Refactor ECS scheduled tasks#1992
theseanything wants to merge 3 commits intomainfrom
theseanything/add-ecs-scheduled-tasks

Conversation

@theseanything
Copy link
Copy Markdown
Contributor

@theseanything theseanything commented Feb 20, 2026

What problem does this pull request solve?

This makes it easier to define multiple ECS Scheduled tasks (e.g. cron jobs). This adds a generic module that defines all the resources to create a scheduled task. This also creates a per account IAM role used by EventBridge scheduler to run tasks.

Will refactor the mailchimp and org sync task in a later PR.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 20, 2026

Pre-Commit report

✅ All pre-commit checks are now passing!


This comment will be updated when code changes.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request refactors ECS scheduled tasks for forms-admin by creating a reusable ecs-scheduled-task module and a shared per-account IAM role for EventBridge. The refactor consolidates duplicate code from the mailchimp-sync and organisations-sync implementations into a generic module that can be used to define multiple scheduled tasks more easily.

Changes:

  • Created a new ecs-scheduled-task module that encapsulates ECS task definition, EventBridge rule, and event target resources
  • Added a shared ecsEventsRole IAM role in the environment module for EventBridge to run ECS tasks
  • Refactored mailchimp-sync and organisations-sync to use the new module with a for_each loop in scheduled-tasks.tf

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
infra/modules/ecs-scheduled-task/main.tf New generic module for creating ECS scheduled tasks with EventBridge triggers
infra/modules/ecs-scheduled-task/variables.tf Input variables for the scheduled task module
infra/modules/ecs-scheduled-task/outputs.tf Output for the EventBridge rule name
infra/modules/forms-admin/scheduled-tasks.tf New file consolidating both scheduled tasks using the generic module with for_each
infra/modules/forms-admin/orgs-sync.tf Removed task definition and EventBridge resources, kept failure monitoring
infra/modules/forms-admin/mailchimp-sync.tf Removed task definition and EventBridge resources, kept failure monitoring
infra/modules/forms-admin/variables.tf Added ecs_events_role_arn variable for the shared IAM role
infra/modules/environment/ecs.tf Created shared ecsEventsRole IAM role for EventBridge
infra/modules/environment/outputs.tf Added output for ecs_events_role_arn
infra/deployments/forms/forms-admin/main.tf Passed ecs_events_role_arn to forms-admin module

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

for_each = local.scheduled_tasks
source = "../ecs-scheduled-task"

task_name = "forms-admin-${replace(each.key, "_", "-")}"
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task name has multiple critical issues:

  1. Pipeline Breakage: The task family name has changed from ${var.environment_name}_forms-admin_mailchimp_sync (e.g., production_forms-admin_mailchimp_sync) to forms-admin-mailchimp-sync. This breaks the deployment pipeline (deploy-forms-admin-container.tf lines 241 and 270) which expects the old naming pattern with underscores and environment prefix. The pipeline's update task definition steps will fail to find the correct tasks.

  2. Logging Issues: The task name is used as the CloudWatch log stream prefix (see ecs-scheduled-task/main.tf line 30). Without the environment name, logs from all environments will have the same prefix (forms-admin-mailchimp-sync/main/*), making it impossible to filter by environment. The old code used forms-admin-${var.env_name}-organisations-sync which included the environment. The troubleshooting documentation in orgs-sync.tf line 43 expects environment-specific log streams.

The task_name should be ${var.env_name}_forms-admin-${replace(each.key, "_", "-")} to match the old pattern and preserve both pipeline compatibility and environment-specific logging.

Suggested change
task_name = "forms-admin-${replace(each.key, "_", "-")}"
task_name = "${var.env_name}_forms-admin-${replace(each.key, "_", "-")}"

Copilot uses AI. Check for mistakes.
Comment on lines +35 to +48
moved {
from = aws_ecs_task_definition.orgs_cron_job[0]
to = module.scheduled_tasks["organisations_sync"].aws_ecs_task_definition.this
}

moved {
from = aws_cloudwatch_event_rule.sync_orgs_cron_job[0]
to = module.scheduled_tasks["organisations_sync"].aws_cloudwatch_event_rule.this
}

moved {
from = aws_cloudwatch_event_target.ecs_org_sync_job[0]
to = module.scheduled_tasks["organisations_sync"].aws_cloudwatch_event_target.this
}
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The moved blocks assume that the old resources existed (i.e., the features were enabled with count=1), but in environments where enable_organisations_sync or enable_mailchimp_sync were false, these resources never existed. When applying this change to those environments, Terraform will try to move non-existent resources and then create new ones, which should work but may cause state inconsistencies. Additionally, since the new module creates resources unconditionally, this means features that were previously disabled will suddenly become enabled in environments like staging and user-research.

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,80 @@
variable "task_name" {
type = string
description = "The scheduled task name suffix."
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description says "The scheduled task name suffix" but this variable is actually used as the complete task family name, not just a suffix. Looking at the usage in main.tf line 5 and line 18 in scheduled-tasks.tf where it's set to "forms-admin-${replace(each.key, "_", "-")}", it's clear this is the full name. The description should be updated to "The complete task family name for the scheduled task" or similar to accurately reflect its usage.

Suggested change
description = "The scheduled task name suffix."
description = "The complete task family name for the scheduled task."

Copilot uses AI. Check for mistakes.
Comment on lines 2 to 3
resource "aws_cloudwatch_event_rule" "sync_orgs_cron_job_failed" {
count = var.enable_organisations_sync ? 1 : 0
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The monitoring resources for orgs sync failures have lost their conditional creation. Previously, these resources had count = var.enable_organisations_sync ? 1 : 0 which prevented them from being created when organisations sync was disabled. Now these resources are always created, which means EventBridge will be monitoring for a task that may not exist in environments where enable_organisations_sync is false. This will create unnecessary resources and potentially confusing monitoring alerts.

Copilot uses AI. Check for mistakes.
Comment on lines 2 to 3
resource "aws_cloudwatch_event_rule" "sync_mailchimp_cron_job_failed" {
name = "${var.env_name}-forms-admin-mailchimp-sync-failed"
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the organisations sync monitoring, the mailchimp sync failure monitoring resources have lost their conditional creation. Previously, these had count = var.enable_mailchimp_sync ? 1 : 0. Now they're always created even when mailchimp sync is disabled in the environment, leading to monitoring of non-existent tasks and unnecessary resource creation.

Copilot uses AI. Check for mistakes.
Comment on lines +14 to +15
module "scheduled_tasks" {
for_each = local.scheduled_tasks
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scheduled tasks are now created unconditionally, but the variables enable_organisations_sync and enable_mailchimp_sync still exist and are being passed to this module. The old implementation used these variables with count to conditionally create the resources. This change means that both scheduled tasks will always be created in all environments, regardless of the variable values, which could lead to unintended task executions in environments where these syncs should be disabled (e.g., staging and user-research environments have both set to false in their tfvars).

Copilot uses AI. Check for mistakes.
@theseanything theseanything force-pushed the theseanything/add-ecs-scheduled-tasks branch 3 times, most recently from 6491073 to 3d5e0a6 Compare February 20, 2026 14:18
This is a shared IAM role for ECS events, this is needed by EventBridge to run ECS scheduled tasks.
@theseanything theseanything force-pushed the theseanything/add-ecs-scheduled-tasks branch from 3d5e0a6 to a8b4967 Compare February 20, 2026 14:20
Introduce a new module for ECS scheduled tasks, including task definition, CloudWatch event rule, and event target configuration. This makes it easy for us to define multiple ECS scheduled tasks.
This provides a place to easily define the schedule tasks.
@theseanything theseanything force-pushed the theseanything/add-ecs-scheduled-tasks branch from a8b4967 to 150486a Compare February 20, 2026 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants